Fix GMCP error caused by forwarding External.Discord.Get to MUME - #244
Fix GMCP error caused by forwarding External.Discord.Get to MUME#244nschimme wants to merge 2 commits into
Conversation
Eat External.Discord.Get in UserTelnet instead of forwarding it to MUME. MUME does not support External.Discord.Get and responds with a MUME.Client.Error, which MMapper was displaying as a protocol error.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRegisters the GMCP message type External.Discord.Get, updates the total GMCP message count, consumes the message in the telnet proxy to avoid MUME protocol errors, and extends tests to cover both External.Discord.Hello and External.Discord.Get handling. Sequence diagram for External.Discord.Get GMCP handlingsequenceDiagram
actor MudClient
participant UserTelnet
participant MUME
MudClient->>UserTelnet: sendExternalDiscordGet()
UserTelnet->>UserTelnet: virt_receiveGmcpMessage(msg)
alt [msg.isExternalDiscordGet()]
UserTelnet-->>MudClient: return (message consumed)
else [other GMCP message]
UserTelnet->>MUME: forwardGmcpMessage(msg)
MUME-->>MudClient: gmcpResponse
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/TestProxy.cpp" line_range="34-42" />
<code_context>
GmcpMessage gmcp3 = GmcpMessage::fromRawBytes(R"(External.Discord.Hello)");
QCOMPARE(gmcp3.getName().toQByteArray(), QByteArray("External.Discord.Hello"));
QVERIFY(!gmcp3.getJson());
+ QVERIFY(gmcp3.isExternalDiscordHello());
+
+ GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
+ QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
+ QVERIFY(!gmcp4.getJson());
+ QVERIFY(gmcp4.isExternalDiscordGet());
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add corresponding serialize test for `External.Discord.Get` to keep deserialize/serialize coverage symmetric.
To fully validate the new GMCP type wiring, please also add a `gmcpMessageSerializeTest` case that builds a `GmcpMessage` of type `ExternalDiscordGet`, serializes it, and verifies the raw bytes (including the name and absence of JSON). This keeps deserialize/serialize tests symmetric and will help catch regressions if the enum or name mapping changes.
Suggested implementation:
```cpp
void TestProxy::gmcpMessageSerializeTest()
{
// existing GMCP serialize tests should remain here
// Symmetric serialize test for External.Discord.Get
GmcpMessage gmcpExternalDiscordGet(/* TODO: use the appropriate constructor or enum for ExternalDiscordGet */);
QByteArray rawExternalDiscordGet = gmcpExternalDiscordGet.toRawBytes();
QCOMPARE(rawExternalDiscordGet, QByteArray("External.Discord.Get"));
QVERIFY(!gmcpExternalDiscordGet.getJson());
}
```
To correctly integrate this change without overwriting existing tests, please:
1. Move the new `GmcpMessage gmcpExternalDiscordGet` block into the existing body of `gmcpMessageSerializeTest()`, alongside the other serialize cases, instead of replacing the whole function. Place it near the serialize test for `External.Discord.Hello` to keep the coverage grouped.
2. Replace the `/* TODO: use the appropriate constructor or enum for ExternalDiscordGet */` with the actual way you construct a `GmcpMessage` of type `ExternalDiscordGet` in your codebase (for example, using the corresponding enum or factory method already used for `ExternalDiscordHello` in the serialize tests).
3. If your serialization API uses a different method than `toRawBytes()` (e.g. `serialize()` or similar), update the call accordingly so that the comparison verifies the exact raw GMCP bytes, matching the format used in the deserialize tests.
</issue_to_address>
### Comment 2
<location path="tests/TestProxy.cpp" line_range="39-42" />
<code_context>
QVERIFY(!gmcp3.getJson());
+ QVERIFY(gmcp3.isExternalDiscordHello());
+
+ GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
+ QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
+ QVERIFY(!gmcp4.getJson());
+ QVERIFY(gmcp4.isExternalDiscordGet());
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that verifies `UserTelnet::virt_receiveGmcpMessage` eats `External.Discord.Get` and does not forward or produce an error.
Right now the tests only cover GMCP deserialization and `isExternalDiscordGet()`. To validate the actual behavior change and prevent regressions, please add or extend a test that exercises `UserTelnet::virt_receiveGmcpMessage` with an `External.Discord.Get` message and asserts there is no GMCP error and no downstream handling (no write to the MUME side, no error callback, etc.).
Suggested implementation:
```cpp
GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
QVERIFY(!gmcp4.getJson());
QVERIFY(gmcp4.isExternalDiscordGet());
}
void TestProxy::externalDiscordGetIsEatenByUserTelnet()
{
// Arrange: set up a UserTelnet instance wired through the proxy test harness.
// This should follow the same pattern as other tests that exercise
// UserTelnet::virt_receiveGmcpMessage in this file.
UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */;
QVERIFY(userTelnet);
// Use the same mechanism other tests use to observe writes to the MUME side
// and GMCP errors (typically QSignalSpy on the appropriate signals).
QSignalSpy mumeWriteSpy(userTelnet, SIGNAL(writeMume(QByteArray)));
QSignalSpy gmcpErrorSpy(userTelnet, SIGNAL(gmcpError(QString)));
// Act: deliver an External.Discord.Get GMCP message to virt_receiveGmcpMessage.
GmcpMessage gmcpGet = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
userTelnet->virt_receiveGmcpMessage(gmcpGet);
// Assert: the message is eaten, no downstream handling and no error.
QCOMPARE(mumeWriteSpy.count(), 0);
QCOMPARE(gmcpErrorSpy.count(), 0);
}
void TestProxy::gmcpMessageSerializeTest()
```
1. At the top of `tests/TestProxy.cpp`, add `#include <QSignalSpy>` if it is not already present to support the new spies.
2. Replace the `UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */;` placeholder with the actual way this test suite obtains a `UserTelnet` instance (for example, using a member like `m_userTelnet`, a helper factory, or existing setup code used by other `virt_receiveGmcpMessage` tests).
3. If the signals used to observe downstream handling differ from `writeMume(QByteArray)` and `gmcpError(QString)` in your codebase, adjust the `QSignalSpy` constructions and the assertions to match the real signal names and parameter types (e.g. `writeToMume`, `gmcpErrorOccurred`, or similar).
4. Register the new test method with Qt’s test system the same way other tests in `TestProxy` are registered (usually nothing extra is needed beyond the member function, but if there is a manual test list or macro, ensure `externalDiscordGetIsEatenByUserTelnet` is included).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| GmcpMessage gmcp3 = GmcpMessage::fromRawBytes(R"(External.Discord.Hello)"); | ||
| QCOMPARE(gmcp3.getName().toQByteArray(), QByteArray("External.Discord.Hello")); | ||
| QVERIFY(!gmcp3.getJson()); | ||
| QVERIFY(gmcp3.isExternalDiscordHello()); | ||
|
|
||
| GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)"); | ||
| QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get")); | ||
| QVERIFY(!gmcp4.getJson()); | ||
| QVERIFY(gmcp4.isExternalDiscordGet()); |
There was a problem hiding this comment.
suggestion (testing): Add corresponding serialize test for External.Discord.Get to keep deserialize/serialize coverage symmetric.
To fully validate the new GMCP type wiring, please also add a gmcpMessageSerializeTest case that builds a GmcpMessage of type ExternalDiscordGet, serializes it, and verifies the raw bytes (including the name and absence of JSON). This keeps deserialize/serialize tests symmetric and will help catch regressions if the enum or name mapping changes.
Suggested implementation:
void TestProxy::gmcpMessageSerializeTest()
{
// existing GMCP serialize tests should remain here
// Symmetric serialize test for External.Discord.Get
GmcpMessage gmcpExternalDiscordGet(/* TODO: use the appropriate constructor or enum for ExternalDiscordGet */);
QByteArray rawExternalDiscordGet = gmcpExternalDiscordGet.toRawBytes();
QCOMPARE(rawExternalDiscordGet, QByteArray("External.Discord.Get"));
QVERIFY(!gmcpExternalDiscordGet.getJson());
}
To correctly integrate this change without overwriting existing tests, please:
- Move the new
GmcpMessage gmcpExternalDiscordGetblock into the existing body ofgmcpMessageSerializeTest(), alongside the other serialize cases, instead of replacing the whole function. Place it near the serialize test forExternal.Discord.Helloto keep the coverage grouped. - Replace the
/* TODO: use the appropriate constructor or enum for ExternalDiscordGet */with the actual way you construct aGmcpMessageof typeExternalDiscordGetin your codebase (for example, using the corresponding enum or factory method already used forExternalDiscordHelloin the serialize tests). - If your serialization API uses a different method than
toRawBytes()(e.g.serialize()or similar), update the call accordingly so that the comparison verifies the exact raw GMCP bytes, matching the format used in the deserialize tests.
| GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)"); | ||
| QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get")); | ||
| QVERIFY(!gmcp4.getJson()); | ||
| QVERIFY(gmcp4.isExternalDiscordGet()); |
There was a problem hiding this comment.
suggestion (testing): Add a test that verifies UserTelnet::virt_receiveGmcpMessage eats External.Discord.Get and does not forward or produce an error.
Right now the tests only cover GMCP deserialization and isExternalDiscordGet(). To validate the actual behavior change and prevent regressions, please add or extend a test that exercises UserTelnet::virt_receiveGmcpMessage with an External.Discord.Get message and asserts there is no GMCP error and no downstream handling (no write to the MUME side, no error callback, etc.).
Suggested implementation:
GmcpMessage gmcp4 = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
QCOMPARE(gmcp4.getName().toQByteArray(), QByteArray("External.Discord.Get"));
QVERIFY(!gmcp4.getJson());
QVERIFY(gmcp4.isExternalDiscordGet());
}
void TestProxy::externalDiscordGetIsEatenByUserTelnet()
{
// Arrange: set up a UserTelnet instance wired through the proxy test harness.
// This should follow the same pattern as other tests that exercise
// UserTelnet::virt_receiveGmcpMessage in this file.
UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */;
QVERIFY(userTelnet);
// Use the same mechanism other tests use to observe writes to the MUME side
// and GMCP errors (typically QSignalSpy on the appropriate signals).
QSignalSpy mumeWriteSpy(userTelnet, SIGNAL(writeMume(QByteArray)));
QSignalSpy gmcpErrorSpy(userTelnet, SIGNAL(gmcpError(QString)));
// Act: deliver an External.Discord.Get GMCP message to virt_receiveGmcpMessage.
GmcpMessage gmcpGet = GmcpMessage::fromRawBytes(R"(External.Discord.Get)");
userTelnet->virt_receiveGmcpMessage(gmcpGet);
// Assert: the message is eaten, no downstream handling and no error.
QCOMPARE(mumeWriteSpy.count(), 0);
QCOMPARE(gmcpErrorSpy.count(), 0);
}
void TestProxy::gmcpMessageSerializeTest()
- At the top of
tests/TestProxy.cpp, add#include <QSignalSpy>if it is not already present to support the new spies. - Replace the
UserTelnet *userTelnet = /* obtain from existing fixture or create as in other tests */;placeholder with the actual way this test suite obtains aUserTelnetinstance (for example, using a member likem_userTelnet, a helper factory, or existing setup code used by othervirt_receiveGmcpMessagetests). - If the signals used to observe downstream handling differ from
writeMume(QByteArray)andgmcpError(QString)in your codebase, adjust theQSignalSpyconstructions and the assertions to match the real signal names and parameter types (e.g.writeToMume,gmcpErrorOccurred, or similar). - Register the new test method with Qt’s test system the same way other tests in
TestProxyare registered (usually nothing extra is needed beyond the member function, but if there is a manual test list or macro, ensureexternalDiscordGetIsEatenByUserTelnetis included).
Eat External.Discord.Get in UserTelnet instead of forwarding it to MUME. MUME does not support External.Discord.Get and responds with a MUME.Client.Error, which MMapper was displaying as a protocol error.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #244 +/- ##
=======================================
Coverage 25.08% 25.09%
=======================================
Files 528 528
Lines 44211 44218 +7
Branches 4793 4796 +3
=======================================
+ Hits 11092 11097 +5
- Misses 33119 33121 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes an issue where MUD clients sending
External.Discord.Getcaused MUME to reply with a GMCP error message that MMapper printed asMUME.Client protocol error: {"message":"unsupported message \"External.Discord.Get\""}.External.Discord.Getis now registered as a GMCP message type and eaten inUserTelnet::virt_receiveGmcpMessage.PR created automatically by Jules for task 5724644234687142926 started by @nschimme
Summary by Sourcery
Handle External.Discord.Get messages locally to avoid MUME GMCP errors.
Bug Fixes:
Enhancements:
Tests: